Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: basic get/post endpoint for v2 xblocks. TNL-10873 #32600

Merged
merged 1 commit into from
Jul 20, 2023

Conversation

kenclary
Copy link
Contributor

@kenclary kenclary commented Jun 29, 2023

feat: basic get/post endpoint for v2 xblocks. TNL-10873

@kenclary kenclary force-pushed the kenclary/TNL-10873 branch 5 times, most recently from 2ea4170 to e00cda1 Compare June 30, 2023 17:30
@kenclary kenclary force-pushed the kenclary/TNL-10873 branch 25 times, most recently from 5b4b53f to 350a379 Compare July 10, 2023 19:30
@kenclary kenclary force-pushed the kenclary/TNL-10873 branch 16 times, most recently from 47b1c70 to 3e03662 Compare July 19, 2023 14:28
@kenclary kenclary changed the title feat: basic get/post endpoint for v2 xblocks (incomplete). TNL-10873 feat: basic get/post endpoint for v2 xblocks. TNL-10873 Jul 19, 2023
@kenclary kenclary requested a review from bradenmacdonald July 19, 2023 14:55
Copy link
Contributor

@bradenmacdonald bradenmacdonald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good! I haven't tested this manually but I'm happy with the code.

openedx/core/djangoapps/xblock/rest_api/views.py Outdated Show resolved Hide resolved
}
}, format='json')
block_saved = xblock_api.load_block(block_key, self.staff_user)
assert block_saved.data == '\n<p>test</p>\n'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it be an issue for the editors if some newlines go inserted here? This test set the data to '<p>test</p>' but we get the data back as '\n<p>test</p>\n'. I'm assuming that's fine but just want to confirm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, in fact I don't think editors can have content without the newlines; it's just an artifact of what I wrote in the test.

@@ -133,7 +133,9 @@ def save_block(self, block):
if not learning_context.can_edit_block(self.user, block.scope_ids.usage_id):
log.warning("User %s does not have permission to edit %s", self.user.username, block.scope_ids.usage_id)
raise RuntimeError("You do not have permission to edit this XBlock")
olx_str, static_files = serialize_xblock(block)
serialized = serialize_modulestore_block_for_blockstore(block)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we should rename this function to just serialize_block_for_blockstore since we're already passing in a blockstore block here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is called in a bunch of other places.

@kenclary kenclary force-pushed the kenclary/TNL-10873 branch from 3e03662 to 255d58b Compare July 19, 2023 20:04
@kenclary kenclary marked this pull request as ready for review July 20, 2023 13:13
@kenclary kenclary force-pushed the kenclary/TNL-10873 branch 2 times, most recently from 7d58c41 to 99f7cdc Compare July 20, 2023 15:20
Copy link
Contributor

@connorhaugh connorhaugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some comments on comments for clarity. Code looks ok.

Comment on lines +179 to +185
View to get/edit the field values of an XBlock as JSON (in the v2 runtime)

This class mimics the functionality of xblock_handler in block.py (for v1 xblocks), but for v2 xblocks.
However, it only implements the exact subset of functionality needed to support the v2 editors (from
the frontend-lib-content-components project). As such, it only supports GET and POST, and only the
POSTing of data/metadata fields.
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
View to get/edit the field values of an XBlock as JSON (in the v2 runtime)
This class mimics the functionality of xblock_handler in block.py (for v1 xblocks), but for v2 xblocks.
However, it only implements the exact subset of functionality needed to support the v2 editors (from
the frontend-lib-content-components project). As such, it only supports GET and POST, and only the
POSTing of data/metadata fields.
"""
View for the field values of an XBlock, returned as JSON.
This class mimics the functionality of the v1 xblock_handler in block.py.
However, it only implements the exact subset of functionality needed to support xblock editing, for a single xblock which has no children blocks. As such, it only supports GET and POST.
"""

openedx/core/djangoapps/xblock/rest_api/views.py Outdated Show resolved Hide resolved
@kenclary kenclary force-pushed the kenclary/TNL-10873 branch from 99f7cdc to d6f824d Compare July 20, 2023 20:03
@kenclary kenclary merged commit 705ff1b into master Jul 20, 2023
@kenclary kenclary deleted the kenclary/TNL-10873 branch July 20, 2023 22:07
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants